Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Block Editor: Validate options for the 'HeadingLevelDropdown' component #65425

Merged
merged 2 commits into from
Sep 26, 2024
Merged

Block Editor: Validate options for the 'HeadingLevelDropdown' component #65425

merged 2 commits into from
Sep 26, 2024

Conversation

philwp
Copy link
Contributor

@philwp philwp commented Sep 17, 2024 •
edited
Loading

Resolves #65422

What?

This PR prevents values that would end up as invalid heading levels from appearing in the HeaderLevelDropdown

Why?

We do not want to allow options that will result in heading tags that do not exist.

How?

The values passed to options are filtered to only allow values that are included in HEADING_LEVELS. This PR still allows 0 as an option as well, in order to handle the case where passing 0 will result in a p tag.

Testing Instructions

  1. Open a post or page.
  2. Insert a heading block via the code editor.
 <!-- wp:heading {"level":4,"levelOptions":[4,5,6,7,"foo"],"className":"wp-block-heading"} -->
<h4 class="wp-block-heading">Markup example</h4>
<!-- /wp:heading -->
  1. Switch to the Visual editor only h4, h5, h6 are available as choices

Testing Instructions for Keyboard

Screenshots or screencast

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 17, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: philwp <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: ndiego <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@philwp
Copy link
Contributor Author

philwp commented Sep 17, 2024

I wonder if we should also add an additional attribute for setting 0 as an allowed value to HeadingLevelDropDown? It should probably default to false, that way it is only necessary to add it to the blocks that can currently handle 0 properly. If we leave my fix the way it is currently implemented, we are avoiding most cases with the incorrect markup, but not all.

@ndiego ndiego added [Package] Components /packages/components [Type] Bug An existing feature does not function as intended [Block] Heading Affects the Headings Block labels Sep 17, 2024
@noisysocks noisysocks added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 18, 2024
@ndiego
Copy link
Member

ndiego commented Sep 26, 2024

@Mamaduka @t-hamano @noisysocks, whenever you have a chance, can you review this implementation? This fix is needed, but I defer to you all the best way to accomplish it. 🙏

Mamaduka
Mamaduka approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@Mamaduka Mamaduka left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @philwp! The fix makes sense and works as expected ✅

Ideally, similar validation should happen at the attribute level, and unsupported values should be discarded, maybe logging a warning in the console.

Example: Updated attribute definition for Heading block:

"levelOptions": {
	"type": "array",
	"items": {
		"enum": [ 1, 2, 3, 4, 5, 6 ],
		"type": "number"
	}
}

packages/block-editor/src/components/block-heading-level-dropdown/index.js
.filter(
( option ) => option === 0 || HEADING_LEVELS.includes( option )
)
.sort( ( a, b ) => a - b ); // Sorts numerically in ascending order;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: While usually sorting callback can be omitted in similar cases, this is safer when dealing with numeric values.

@Mamaduka Mamaduka added the props-bot Adding this label triggers the Props Bot workflow for a PR. label Sep 26, 2024
@github-actions github-actions bot removed the props-bot Adding this label triggers the Props Bot workflow for a PR. label Sep 26, 2024
@Mamaduka Mamaduka changed the title Invalid heading levels are now removed from the HeadingLevelDropdown … Block Editor: Validate options for the 'HeadingLevelDropdown' component Sep 26, 2024
@Mamaduka Mamaduka merged commit f624423 into WordPress:trunk Sep 26, 2024
69 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 26, 2024
gutenbergplugin pushed a commit that referenced this pull request Sep 26, 2024
@philwp @Mamaduka
@ndiego @gutenbergplugin
Block Editor: Validate options for the 'HeadingLevelDropdown' compone…
d6dfb79 
…nt (#65425)


Co-authored-by: philwp <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: ndiego <[email protected]>
@github-actions github-actions bot added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Sep 26, 2024
@github-actions GitHub Actions
Copy link

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: d6dfb79

@noisysocks noisysocks mentioned this pull request Sep 30, 2024
Closed
@draganescu draganescu mentioned this pull request Oct 15, 2024
Merged
@sirreal sirreal mentioned this pull request Oct 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mamaduka Mamaduka Mamaduka approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix ellatrix is a code owner

@noisysocks noisysocks Awaiting requested review from noisysocks

@t-hamano t-hamano Awaiting requested review from t-hamano

Assignees
No one assigned
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Block] Heading Affects the Headings Block [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
WordPress 6.7 Editor Tasks
Status: ✅ Done
Milestone
Gutenberg 19.4
Development

Successfully merging this pull request may close these issues.

The HeadingLevelDropdown should only display valid heading levels
4 participants
@philwp @ndiego @Mamaduka @noisysocks